-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Provide explicit version ranges for dependencies #79
Provide explicit version ranges for dependencies #79
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I didn't actually know that pip could specify ranges, but I like them, they allow being very clear about what the requirements are.
One thing that we usually do is to include a section in PRs for testing or verification instructions; this helps the reviewer to quickly get up to speed in case there is setup that's required in order to be able to test. In this particular case, I think the Travis success is enough of a test all by itself, so it's just something that would be helpful once you start in on the reference implementation since I won't have any idea how anything works at that point. 😄
That's great to know, thanks @ddohler! I noticed that section in the DRIVER PR template but forgot about it here. Next time 👍 One thing I've been thinking about: before pulling this in, would it make sense to tag c55ce26 as v0.1 of Ashlar and pin the DRIVER app dependencies to that tag? I doubt that this PR will mess anything up, but going forward it'd be nice to be able to push changes to Ashlar with the confidence that we won't accidentally interfere with DRIVER development. |
Yeah, I was thinking about that too. At first my thought was that this has a low likelihood of breaking things and the deployed versions of DRIVER use pinned Docker images, so I was thinking about just merging this and then fixing any fallout before we deploy a new version of DRIVER. However, we do have at least one person who's building his own Docker images and I'm worried about potentially causing breakage for him. However, I'm also hesitant to tag known-broken code as a "release" because I don't want to give the impression that anyone should use it. What do you think of tagging the current state of things with a non-numeric version like "legacy" or something like that and then pointing DRIVER at that tag? Going forward, we should definitely start something more rigorous; our usual way of doing things is to have releases on How does that all sound? |
I like that idea! To be totally clear, I'm interpreting the steps you're suggesting to be:
This sounds like a reasonably safe and sane way to move forward. One thought: I wonder if the user building their own images will get the benefit of the pin to |
Yup, that's all correct. Oops, yes, you're right that this would still affect legacy DRIVER users until they updated. I think what you suggest (notifying them, because it's just the one guy) is probably the best plan, although if we do that then we should probably test against a local version of DRIVER (by changing the dependencies to point to this branch and then reprovisioning) to confirm this doesn't cause breakage. Once we merge this I can just send him a ping to be on the lookout for problems. |
Alright, tested this locally and it seems fine! Instead of doing a full reprovision, I went ahead and rebuilt the container that relies on Ashlar ( I also pulled down these changes into the sister |
Awesome, thanks for doing that. I think this is good to go; I'll merge this in and do the other steps along with merging. |
What's the status on this @ddohler? Seems like the only open issue remaining from azavea/grout-2018-fellowship#1. |
Holding for WorldBank-Transport/DRIVER#690 to get merged; I've notified Tiago so I think once that's merged the rest can happen pretty quickly. |
Looks like WorldBank-Transport/DRIVER#690 is merged in! Can I pull this in @ddohler? |
Yep, go for it! Can you also take care of steps 3-6 in your message above along with that? |
I'd be happy to 👍 |
Superceded by #81. |
Since Travis last tested this project in May of 2016, many of the dependencies required in
setup.py
have published new major versions with breaking changes. Travis has also undergone a major upgrade to Ubuntu 14.04, which has changed some configuration options.This PR updates the project requirements to reflect the actual range of possible dependency versions for the app as it currently exists, and adjusts
.travis.yml
to get builds working again.